Handle redirects in apiserver proxy handler #34987

timstclair · 2016-10-17T21:57:29Z

Overview:

Peek at the HTTP response from the proxied backend
If it is a redirect response (302/3), redo the request to the redirect location
If it's not a redirect, forward the response to the client and then set up the proxy as before

This change is required for implementing streaming requests in the Container Runtime Interface (CRI). See design.

For #29579

/cc @yujuhong

This change is

ncdc · 2016-10-24T15:03:22Z

I'll try to take a look at this later today or tomorrow. Also cc @liggitt

liggitt · 2016-10-24T15:16:51Z

need to take a closer look, but at the very least, this should be opt-in... not all proxy handling should follow redirects server-side

timstclair · 2016-10-24T18:23:08Z

at the very least, this should be opt-in... not all proxy handling should follow redirects server-side

Makes sense. Here's a couple options:

Make it configurable in the proxy, e.g. configure the proxy for exec/attach/portforward to follow redirects
Make it configurable per-request/response, e.g. a header specifies whether the proxy should follow the redirect
I suppose the traditional solution from a reverse proxy perspective would be to not handle the redirect at all, and just rewrite the location header with the external address (i.e. /proxy/...)

I'm leaning towards option (1), since the apiserver can expect the redirect in that case. Option (3) could work, but it would add an extra hop to the connection (might be a non-issue?), and the apiserver would need to know if redirect location was a pod or a node.

Am I missing anything?

/cc @yujuhong

liggitt · 2016-10-24T18:49:25Z

option 1, feature-gated with CRI-enablement, seems like the best approach. still want to dig into the mechanism (and ideally factor it out of the main flow a little better)

yujuhong · 2016-10-24T19:22:02Z

option 1, feature-gated with CRI-enablement, seems like the best approach. still want to dig into the mechanism (and ideally factor it out of the main flow a little better)

Option 1 sounds good to me, too.

I don't think we need to feature gate this in the apiserver. CRI affects how kubelet integrates with the runtime, and should be feature gated there (on the kubelet side). This should not concern apiserver, and apiserver should not receive redirects (for exec/attach/portforward) at all if the feature is not enabled in kubelet.

liggitt · 2016-10-24T19:23:29Z

I don't think we need to feature gate this in the apiserver

sniffing responses and redirecting is a pretty invasive change (as evidenced by this PR)... it should be gated to protect against issues found while the feature is in beta

timstclair · 2016-10-26T02:09:30Z

I refactored this based on the discussion here. Redirect interception is now only enabled for exec/attach/port-forward requests, and can be disabled with a feature gate (StreamingProxyRedirects).

liggitt · 2016-10-27T04:22:57Z

pkg/registry/generic/rest/proxy.go

 	if err != nil {
 		return nil, err
 	}
 	removeCORSHeaders(resp)
 	return resp, nil
 }

-var _ = net.RoundTripperWrapper(&corsRemovingTransport{})


don't drop this, let the compiler help us

The compiler already enforces this since we use it in a net.RoundTripperWrapper context, and the net package name now conflicts with the stdlib net. If you feel strongly I can alias the package name and add it back.

I'm not seeing the type assertion elsewhere, I'd like to keep it

liggitt · 2016-10-27T04:23:38Z

pkg/util/config/feature_gate.go

 )

 var (
 	// Default values for recorded features.  Every new feature gate should be
 	// represented here.
 	knownFeatures = map[string]featureSpec{
 		allAlphaGate:              {false, alpha},
-		externalTrafficLocalOnly:  {false, alpha},
+		externalTrafficLocalOnly:  {true, beta},


Just something weird about the diffing... I rebased on HEAD and this resolved.

liggitt · 2016-10-27T04:30:17Z

pkg/registry/generic/rest/proxy.go

-
-	backendConn, err := proxy.DialURL(h.Location, h.Transport)
-	if err != nil {
+	if err := h.handleUpgrade(w, req); err != nil {


This change moves the hijack and close of the request connection before we get a chance to respond to errors dialing the backend. That means we would no longer send API error responses in error cases.

Good catch! It turns out this is an existing issue, since the ErrorResponder fails once the connection is hijacked. I added a test case for this, and fixed it by writing the errors to the hijacked connection directly.

timstclair

Thanks for reviewing! Responded to all comments.

timstclair · 2016-10-28T00:17:35Z

pkg/registry/generic/rest/proxy.go

-
-	backendConn, err := proxy.DialURL(h.Location, h.Transport)
-	if err != nil {
+	if err := h.handleUpgrade(w, req); err != nil {


Good catch! It turns out this is an existing issue, since the ErrorResponder fails once the connection is hijacked. I added a test case for this, and fixed it by writing the errors to the hijacked connection directly.

timstclair · 2016-10-28T00:19:27Z

pkg/registry/generic/rest/proxy.go

 	if err != nil {
 		return nil, err
 	}
 	removeCORSHeaders(resp)
 	return resp, nil
 }

-var _ = net.RoundTripperWrapper(&corsRemovingTransport{})


The compiler already enforces this since we use it in a net.RoundTripperWrapper context, and the net package name now conflicts with the stdlib net. If you feel strongly I can alias the package name and add it back.

timstclair · 2016-10-28T00:41:46Z

pkg/util/config/feature_gate.go

 )

 var (
 	// Default values for recorded features.  Every new feature gate should be
 	// represented here.
 	knownFeatures = map[string]featureSpec{
 		allAlphaGate:              {false, alpha},
-		externalTrafficLocalOnly:  {false, alpha},
+		externalTrafficLocalOnly:  {true, beta},


Just something weird about the diffing... I rebased on HEAD and this resolved.

liggitt · 2016-10-28T01:12:28Z

It turns out this is an existing issue, since the ErrorResponder fails once the connection is hijacked. I added a test case for this, and fixed it by writing the errors to the hijacked connection directly.

The previous flow was intentional... let the error responder report errors during backend establishment, then hijack. The error responder can't write once we've written content to the connection (headers are already committed, and a well formed API error JSON blob after random other content isn't usable)

liggitt · 2016-10-28T01:14:13Z

pkg/registry/generic/rest/proxy.go

+func (r *hijackedErrorResponder) Error(err error) {
+	header := http.Header{}
+	header.Set("Content-Type", "text/plain")
+	body := bytes.NewBufferString(err.Error())


Wasn't this JSON before (or maybe negotiated?)

timstclair · 2016-10-28T01:22:31Z

The previous flow was intentional... let the error responder report errors during backend establishment, then hijack.

Hmm, I see. I'll refactor it a bit tomorrow wait until after the BE connection is established to hijack.

There was still an error in the old flow though, because the Responder was invoked after the connection was hijacked (here). I guess the hijacking should just be moved so that it's the last step before the proxy is established?

liggitt · 2016-10-28T01:25:29Z

Yeah, the request construction block might be able to be moved above the hijack. Yeah, hijack at the last possible moment

timstclair · 2016-10-28T20:05:02Z

Done. PTAL.

liggitt · 2016-10-31T02:51:42Z

Will finish up review tomorrow

yujuhong · 2016-11-01T20:32:52Z

@liggitt @ncdc a friendly ping since code freeze is imminent!

ncdc · 2016-11-01T20:41:46Z

pkg/registry/generic/rest/proxy.go

 	if err != nil {
-		h.Responder.Error(err)
+		h.Responder.Error(fmt.Errorf("error dialing backend: %v", err))


Depending on what causes connectBackend/connectBackendWithRedirects to return an error, you may end up writing error dialing backend: error dialing backend: .... It would be nice to avoid that if possible.

ncdc · 2016-11-01T20:52:13Z

pkg/registry/generic/rest/proxy.go

-		return true
+	// Forward raw response bytes back to client.
+	if _, err = requestHijackedConn.Write(rawResponse); err != nil {
+		glog.Errorf("Error proxying response from backend to client: %v", err)


utilruntime.HandleError(fmt.Errorf("error proxying response...

Done (What does this give us? Should I update the other error logging in the go routines below too?)

liggitt · 2016-11-01T21:07:52Z

pkg/registry/generic/rest/proxy.go

+		return conn, fmt.Errorf("error dialing backend: %v", err)
+	}
+
+	if err = beReq.Write(conn); err != nil {


doesn't this consume and close the original req.Body?

it proxies the original request's body to the backend. So that's the intention, isn't it?

yes, but connectBackend() is called repeatedly with the same request in cases where the backend returns a redirect

liggitt · 2016-11-01T21:08:41Z

pkg/registry/generic/rest/proxy.go

+			return nil, nil, fmt.Errorf("too many redirects (%d)", redirects)
+		}
+
+		intermediateConn, err = h.connectBackend(req, location)


doesn't calling connectBackend consume the req.Body the first time? won't that cause failures in subsequent calls (or in the final call when the CRI redirect destination doesn't get any body content?)

Hmm, I think so, but this is also consistent with what http.Client.Do does. That implementation changes the redirected requests to GET (from POST) requests though, and I bet this is why. This shouldn't affect our usage, since these requests don't have bodies anyway. What do you think the best way to deal with it is?

I was going to ask the same thing. Maybe we need a test with multiple redirects?

Thinking about this a bit more... If we decide to move to HTTP/2 for streaming requests, and if there are no changes to go's HTTP/2 library, then the only means we'll have of streaming data over the wire will be via the request and response bodies (we'd need to implementing muxing on top). Which would mean that when the client has data it wants to send to the server, the original request body needs to be preserved and available. I'm not sure how that would work... Also not sure it needs to stop this from going in.

Can't we buffer a limited amount of the body and re-send it again and again? Like n<=1000 bytes. Every server should know what to do after those n bytes. If it asks for more, we continue to assume that there is no redirect.

ncdc · 2016-11-01T21:41:02Z

I would feel more comfortable with something like this at the beginning of the dev cycle as opposed to just before code freeze. We may need to be prepared to revert and redo if things start behaving oddly.

timstclair

Thanks, addressed comments. Open question about handling request body in redirects.

I would feel more comfortable with something like this at the beginning of the dev cycle as opposed to just before code freeze. We may need to be prepared to revert and redo if things start behaving oddly.

Ack. There is a feature flag if need be, and this code path shouldn't be exercised normally anyway. Also, now is just before feature freeze, we still have a few more weeks of bug fixing, testing and stabilization.

timstclair · 2016-11-01T21:16:25Z

pkg/registry/generic/rest/proxy.go

 	if err != nil {
-		h.Responder.Error(err)
+		h.Responder.Error(fmt.Errorf("error dialing backend: %v", err))


timstclair · 2016-11-01T21:19:04Z

pkg/registry/generic/rest/proxy.go

-		return true
+	// Forward raw response bytes back to client.
+	if _, err = requestHijackedConn.Write(rawResponse); err != nil {
+		glog.Errorf("Error proxying response from backend to client: %v", err)


Done (What does this give us? Should I update the other error logging in the go routines below too?)

timstclair · 2016-11-01T21:26:04Z

pkg/registry/generic/rest/proxy.go

+			return nil, nil, fmt.Errorf("too many redirects (%d)", redirects)
+		}
+
+		intermediateConn, err = h.connectBackend(req, location)


Hmm, I think so, but this is also consistent with what http.Client.Do does. That implementation changes the redirected requests to GET (from POST) requests though, and I bet this is why. This shouldn't affect our usage, since these requests don't have bodies anyway. What do you think the best way to deal with it is?

timstclair · 2016-11-01T21:36:07Z

pkg/registry/generic/rest/proxy.go

 	if err != nil {
 		return nil, err
 	}
 	removeCORSHeaders(resp)
 	return resp, nil
 }

-var _ = net.RoundTripperWrapper(&corsRemovingTransport{})


liggitt · 2016-11-03T19:22:58Z

pkg/registry/generic/rest/proxy.go

+
+	conn, err = proxy.DialURL(location, h.Transport)
+	if err != nil {
+		return conn, fmt.Errorf("error dialing backend: %v", err)


return nil in error scenarios? otherwise we try to double close it, right? (in the defer here, and in the defer in connectBackendWithRedirects())

Good catch. Done.

liggitt · 2016-11-03T19:54:21Z

pkg/registry/generic/rest/proxy.go

+	defer func() {
+		if err != nil && conn != nil {
+			conn.Close()
+			conn = nil


assigning nil here doesn't change the returned value (https://play.golang.org/p/q5dnDAYsL6). actually do return nil, ... in error cases.

edit: I'm wrong, named returns make this work correctly. your call whether you want to switch it to clean up inline and return nil

liggitt · 2016-11-03T20:08:34Z

this LGTM, go ahead and squash

we should keep the redirection length limitation issues in mind when continuing the CRI design...

liggitt · 2016-11-03T20:19:57Z

hmmm

go test -v k8s.io/kubernetes/pkg/registry/generic/rest -run TestProxyUpgrade$
proxy_test.go:437: http with redirect: websocket dial err: websocket.Dial ws://127.0.0.1:60163/some/path: bad status

k8s-ci-robot · 2016-11-03T20:52:21Z

Jenkins GCI GCE e2e failed for commit fa825d9. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

timstclair · 2016-11-03T20:54:08Z

Oops, I forgot to flip the feature flag back on for the test.

timstclair · 2016-11-03T21:15:21Z

Filed #36187 to track follow up work.

timstclair · 2016-11-03T21:33:51Z

Squashed.

k8s-ci-robot · 2016-11-03T22:03:43Z

Jenkins unit/integration failed for commit 389a54551c70a38611a4efbab6056ae7a950b23e. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-11-03T22:12:01Z

Jenkins verification failed for commit 389a54551c70a38611a4efbab6056ae7a950b23e. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

timstclair · 2016-11-03T22:44:27Z

@k8s-bot unit test this #32455

timstclair · 2016-11-03T22:45:06Z

Reran hack/update-bazel.sh

sttts · 2016-11-04T10:45:18Z

pkg/registry/generic/rest/proxy.go

+
+redirectLoop:
+	for redirects := 0; ; redirects++ {
+		if redirects == maxRedirects {


Off by one. With maxRedirects==0 we should at least connect once.

follow up for this is fine

Thanks, fixed.

timstclair · 2016-11-04T19:28:13Z

Fixed off-by-one. Reapplying LGTM.

yujuhong · 2016-11-05T00:53:11Z

Marking p2 to ensure PR dependency is respected.

k8s-ci-robot · 2016-11-05T11:17:19Z

Jenkins GCE e2e failed for commit 6e0702a. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

timstclair · 2016-11-05T19:30:48Z

@k8s-bot cvm gce e2e test this #33380

k8s-github-robot · 2016-11-05T21:58:26Z

Automatic merge from submit-queue

@euank

Automatic merge from submit-queue Use indirect streaming path for remote CRI shim Last step for #29579 - Wire through the remote indirect streaming methods in the docker remote shim - Add the docker streaming server as a handler at `<node>:10250/cri/{exec,attach,portforward}` - Disable legacy streaming for dockershim Note: This requires PR #34987 to work. Tested manually on an E2E cluster. /cc @euank @feiskyer @kubernetes/sig-node

timstclair added the release-note-none Denotes a PR that doesn't merit a release note. label Oct 17, 2016

timstclair added this to the v1.5 milestone Oct 17, 2016

timstclair assigned ncdc Oct 17, 2016

googlebot added the cla: yes label Oct 17, 2016

k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 17, 2016

ncdc assigned liggitt Oct 24, 2016

liggitt reviewed Oct 27, 2016

View reviewed changes

timstclair force-pushed the redirect branch from 60322a1 to 814ebad Compare October 28, 2016 00:41

timstclair commented Oct 28, 2016

View reviewed changes

liggitt reviewed Oct 28, 2016

View reviewed changes

ncdc reviewed Nov 1, 2016

View reviewed changes

liggitt reviewed Nov 1, 2016

View reviewed changes

timstclair commented Nov 1, 2016

View reviewed changes

liggitt reviewed Nov 3, 2016

View reviewed changes

timstclair mentioned this pull request Nov 3, 2016

[CRI] Avoid user data in exec redirect URL #36187

Closed

timstclair force-pushed the redirect branch from 1863285 to 389a545 Compare November 3, 2016 21:21

timstclair force-pushed the redirect branch from 389a545 to c64276b Compare November 3, 2016 22:44

sttts reviewed Nov 4, 2016

View reviewed changes

timstclair mentioned this pull request Nov 4, 2016

Use indirect streaming path for remote CRI shim #36253

Merged

liggitt added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2016

Handle redirects in apiserver proxy handler

6e0702a

timstclair force-pushed the redirect branch from c64276b to 6e0702a Compare November 4, 2016 19:26

timstclair added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Nov 4, 2016

yujuhong added the priority/backlog Higher priority than priority/awaiting-more-evidence. label Nov 5, 2016

k8s-github-robot merged commit 7d1ef3e into kubernetes:master Nov 5, 2016

Handle redirects in apiserver proxy handler #34987

Handle redirects in apiserver proxy handler #34987

Conversation

timstclair commented Oct 17, 2016 • edited

ncdc commented Oct 24, 2016

liggitt commented Oct 24, 2016

timstclair commented Oct 24, 2016

liggitt commented Oct 24, 2016

yujuhong commented Oct 24, 2016

liggitt commented Oct 24, 2016 • edited

timstclair commented Oct 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timstclair left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Oct 28, 2016

Choose a reason for hiding this comment

timstclair commented Oct 28, 2016

liggitt commented Oct 28, 2016

timstclair commented Oct 28, 2016

liggitt commented Oct 31, 2016

yujuhong commented Nov 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Nov 1, 2016 • edited

Choose a reason for hiding this comment

sttts Nov 3, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Nov 1, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncdc commented Nov 1, 2016

timstclair left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Nov 3, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Nov 3, 2016 • edited

Choose a reason for hiding this comment

liggitt commented Nov 3, 2016 • edited

liggitt commented Nov 3, 2016

k8s-ci-robot commented Nov 3, 2016

timstclair commented Nov 3, 2016

timstclair commented Nov 3, 2016

timstclair commented Nov 3, 2016

k8s-ci-robot commented Nov 3, 2016

k8s-ci-robot commented Nov 3, 2016

timstclair commented Nov 3, 2016

timstclair commented Nov 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timstclair commented Nov 4, 2016

yujuhong commented Nov 5, 2016

k8s-ci-robot commented Nov 5, 2016

timstclair commented Nov 5, 2016

k8s-github-robot commented Nov 5, 2016

timstclair commented Oct 17, 2016 •

edited

liggitt commented Oct 24, 2016 •

edited

liggitt Nov 1, 2016 •

edited

sttts Nov 3, 2016 •

edited

liggitt Nov 1, 2016 •

edited

liggitt Nov 3, 2016 •

edited

liggitt Nov 3, 2016 •

edited

liggitt commented Nov 3, 2016 •

edited